-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[HOLD discussion] Add section for Proposing Performance Improvements #78757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@Krishna2323 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
contributingGuides/PERFORMANCE.md
Outdated
| *To ensure proposals are measurable and based on realistic scenarios, you must meet the following criteria:* | ||
| - [ ] **Experience:** I have at least **1 merged PR** in the App repository. | ||
| - [ ] **Test Environment:** I tested on a high-traffic account (instructions to create this [here](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#high-traffic-accounts)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still deciding if we should go with a high traffic account, OR link to some cleaned onyx state for download
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having a real online account is better so it is closer to real-life user experience, however, I think we might need a bit more complex account than the high-traffic account. It does not have any transactions/ approvers etc as far as I know. Maybe we could beef it up with some script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with both things Vit said.
We also want to be careful that this demo account doesn't become outdated as we release more and more functionality. Just something to keep in mind, don't need to solve it right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the plan to use a "real online account" instead of purely importing onyx state, and yeah maybe a follow-up to getting this running will be to figure out how to beef up accounts for many-transactions / many-workspace-members / etc?
contributingGuides/PERFORMANCE.md
Outdated
| *To ensure proposals are measurable and based on realistic scenarios, you must meet the following criteria:* | ||
| - [ ] **Experience:** I have at least **1 merged PR** in the App repository. | ||
| - [ ] **Test Environment:** I tested on a high-traffic account (instructions to create this [here](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#high-traffic-accounts)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having a real online account is better so it is closer to real-life user experience, however, I think we might need a bit more complex account than the high-traffic account. It does not have any transactions/ approvers etc as far as I know. Maybe we could beef it up with some script?
contributingGuides/PERFORMANCE.md
Outdated
| *To ensure proposals are measurable and based on realistic scenarios, you must meet the following criteria:* | ||
| - [ ] **Experience:** I have at least **1 merged PR** in the App repository. | ||
| - [ ] **Test Environment:** I tested on a high-traffic account (instructions to create this [here](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#high-traffic-accounts)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with both things Vit said.
We also want to be careful that this demo account doesn't become outdated as we release more and more functionality. Just something to keep in mind, don't need to solve it right now.
Co-authored-by: Vit Horacek <[email protected]>
Co-authored-by: Tim Golen <[email protected]> Co-authored-by: Vit Horacek <[email protected]>
…ify/App into beaman-addPerformanceProposalPlan
…manceProposalPlan
contributingGuides/PERFORMANCE.md
Outdated
| ___ | ||
| If you haven't already, check out our [Contributing Guidelines](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md) and email [email protected] to request access to our Slack channel! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and email [email protected] to request access to our Slack channel!
We handle this via a google form now. Easiest to just delete this text cuz it's already in CONTRIBUTING.md . If you want to keep, update to
If you would like to join our #expensify-open-source Slack channel, fill out this form with your email and Upwork profile link. If you haven't been added in 2 weeks, email [email protected].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh ok well maybe I can just keep the first part (check out our contributing guidelines) but remove the part about requesting slack channel access? I think it's always a good reminder & good practice to link to our main CONTRIBUTING docs - but we don't have to explain any processes here that are found there, as you said!
contributingGuides/PERFORMANCE.md
Outdated
| | **Render Count** | | | | | ||
| | **Execution Time** | | | | | ||
| | **Perceived Latency** | | | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of a table we could use some tool and upload a JSON in the comment? Just to make sure that contributors don't choose only "the best numbers".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's totally fair, what tool would you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general in this case I think the current version can stay as it is to not cause much delay, but I think it's worth to consider the options that we may have!
Internally we're using a tool that our colleague @sumo-slonik has built to compare the commit number and render times from the gathered profile traces. I think the contributors could simply add a screenshot from the tool and the profile traces that were used to generate it. It works great for the component performance, although with a single function execution time it might be a bit more tricky 😄
I usually rely on SpeedScope for analyzing web performance traces, but I’m curious whether others have a faster or more reliable way to measure function execution time—for example, across 100 runs—without significantly modifying the code? 😄 cc: @adhorodyski
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a few adjustments - but yeah i think we can improve this as we go, for now let's keep it as is with the new format, unless y'all feel strongly for now!
Co-authored-by: Carlos Martins <[email protected]>
Co-authored-by: Carlos Martins <[email protected]>
…manceProposalPlan
…ify/App into beaman-addPerformanceProposalPlan
|
Alright y'all thanks so much for all the comments for now! How are we feeling about the current version? We can absolutely change it more in the future, but it would be nice to get this out in the real world soon to see what kind of proposals we get! |
Co-authored-by: Carlos Martins <[email protected]>
fabioh8010
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for joining the discussions late, was OOO 🙈 The changes look really great so far! I post some small comments however
| ## 4. Prerequisites & Eligibility | ||
| *To ensure proposals are measurable and based on realistic scenarios, you must meet the following criteria:* | ||
| - [ ] **Test Environment:** I tested on a high-traffic account (instructions to create this [here](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#high-traffic-accounts)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we agreed to use a curated onyx state file obtained from the heavy applause account? https://expensify.slack.com/archives/C05LX9D6E07/p1765376829013879
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah in slack we left it there, but in this PR (here) we discussed and thought it would be better to go with a live account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we are going abck and forth there but we also seen that offline, lots of events get hidden the performance offline is not exactly what we get when user is online, so we would like to use account that is online preferably
| *Device Used:* (e.g. iPhone 13, Pixel 6, Chrome on M1 Mac) - Note: Don't use CPU throttling for these measurements! | ||
| - Device CPU: ___ | ||
| - Device RAM: ___ | ||
| *Evidence:* (Attach screenshots of the profiler or logs for both Before and After below this section)* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these attachments/logs be attached inside the thread of the Slack message? If yes I think we should be clear about it.
For example, "below this section" won't work in Slack like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean clear that it should be the thread and not the main post? It looks clear to me that is should be attached
| ### Instructions for Submission | ||
| 1. Copy the template below. | ||
| 2. Fill out the details strictly following the guide. | ||
| 3. Post it in `#expensify-open-source` with the title `[Performance Proposal] <Component_Name>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title should be included as the header of the Slack message content
| ___ | ||
| ``` | ||
| ## 1. Component and Flow Description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ## 1. Component and Flow Description | |
| [Performance Proposal] <Component_Name> | |
| ## 1. Component and Flow Description |
Example
| *Performance improvements should not change user experience and product design.* | ||
| - [ ] This change preserves existing UX (No visual/behavioral changes). | ||
| - [ ] This change alters UX (Description: _________________). | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.


Explanation of Change
Creates template for contributors to propose performance improvements in our App product
Fixed Issues
$ #66161
Tests
None needed, this just adds documentation
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari